-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simply retry strategy for minor errors #3380
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
|
this.state = { | ||
kind: "FAILED", | ||
durationMs: this.calculateDuration(), | ||
error, | ||
errorType, | ||
message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some awkwardness here that makes me want to suggest exceptions as a failure mechanism.
I remember you don't like exceptions, so I'll explain my observations first.
- You log the error here (reasonable), but then sometimes you log it again, e.g. in
FixCodeUntilItRuns
:
context.fail("MINOR", newCode.value);
context.log({
type: "codeRunError",
error: newCode.value,
});
This will lead to duplicate error log entries.
Because you needed to type it as codeRunError
... ok, one way around it is to pass the error to fail()
, like, fail(errorType: ErrorType, message: Extract<LogEntry, { type: "error" | "codeRunError" }>)
, and then pass that. Fixable.
- But then there's also the awkwardness that the step code can call
fail()
and then continue to do things. Which would be mostly ignored: the step is already failed, there should be nothing to do.
This also interferes with LLMStep.run()
: you had to explicitly cast this.state as CodeState
there, etc.
So, how about we just use throw MinorStepError(message)
as the main way to fail? Then run()
becomes more natural, if we catch an error then we minor-fail and either log the message or log String(error)
if it's not a minor error. And we complete()
otherwise.
One reason why I like this is because we already support exceptions; steps can throw, and we don't ask step implementations to carefully try/catch their code. So they might as well throw more meaningful errors.
In other words, fail("MINOR", "message")
function is unnecessary, it's identical to throw new Error("message")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will experiment here.
This closes: #3375
Also, this does minor visual updates